You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Thanks for contributing to Selenium! A PR well described will help maintainers to quickly review and merge it
Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.
Description
This pull request includes several changes to improve exception messages and modify collection handling in the Selenium Java codebase. The most important changes include updating exception messages to use this instead of toString() and replacing Collectors.toSet() with Collectors.toUnmodifiableSet().
Here are some key observations to aid the review process:
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review
Potential Performance Impact The change from Collectors.toSet() to Collectors.toUnmodifiableSet() might have performance implications. While it provides immutability, it could potentially be slower for large collections. This should be validated, especially if this method is called frequently.
-for (By by : bys) {- List<WebElement> elements = context.findElements(by);- if (!elements.isEmpty()) {- return elements.get(0);- }-}-throw new NoSuchElementException("Cannot locate an element using " + this);+return Arrays.stream(bys)+ .map(context::findElements)+ .flatMap(List::stream)+ .findFirst()+ .orElseThrow(() -> new NoSuchElementException("Cannot locate an element using " + this));
Apply this suggestion
Suggestion importance[1-10]: 8
Why: This suggestion significantly enhances the code by using Java 8 streams and Optional, making the logic more concise and expressive. It also potentially increases safety by handling the absence of elements more gracefully.
8
Simplify the stream operation by using a method reference instead of a lambda expression
Consider using a method reference instead of a lambda expression for the map operation. This can make the code more concise and potentially easier to read.
Why: The suggestion to use a method reference instead of a lambda expression improves code readability and conciseness without altering functionality. This is a good practice in Java, making the code more idiomatic and easier to maintain.
7
Possible issue
Add a safeguard against potential null or empty list access
Consider adding a null check for the allElements list before accessing its first element to prevent potential NullPointerException.
if (allElements == null || allElements.isEmpty()) {
throw new NoSuchElementException("Cannot locate an element using " + this);
}
-return allElements.get(0);+return allElements.stream().findFirst().orElseThrow(() -> new NoSuchElementException("Element list is empty"));
Apply this suggestion
Suggestion importance[1-10]: 6
Why: The suggestion to use a stream operation with orElseThrow adds an additional safeguard against null or empty list access, which can prevent runtime exceptions and improve code robustness.
6
Best practice
Improve code structure by using an early return pattern and adding braces for clarity
Consider using an early return statement to improve code readability and reduce nesting.
public WebElement findElement(SearchContext context) {
List<WebElement> elements = findElements(context);
- if (elements.isEmpty())+ if (elements.isEmpty()) {
throw new NoSuchElementException("Cannot locate an element using " + this);
+ }
return elements.get(0);
}
Apply this suggestion
Suggestion importance[1-10]: 5
Why: The suggestion to add braces improves code readability and reduces potential errors related to missing braces in future modifications. However, the early return pattern is already effectively used, so the impact is moderate.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
iampopovich
changed the title
User description
Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it
Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.
Description
This pull request includes several changes to improve exception messages and modify collection handling in the Selenium Java codebase. The most important changes include updating exception messages to use
thisinstead oftoString()and replacingCollectors.toSet()withCollectors.toUnmodifiableSet().Improvements to exception messages:
java/src/org/openqa/selenium/By.java: UpdatedNoSuchElementExceptionmessage to usethisinstead oftoString()in thefindElementmethod.java/src/org/openqa/selenium/support/pagefactory/ByAll.java: UpdatedNoSuchElementExceptionmessage to usethisinstead oftoString()in thefindElementmethod.java/src/org/openqa/selenium/support/pagefactory/ByChained.java: UpdatedNoSuchElementExceptionmessage to usethisinstead oftoString()in thefindElementmethod.Changes to collection handling:
java/src/org/openqa/selenium/JavascriptExecutor.java: ReplacedCollectors.toSet()withCollectors.toUnmodifiableSet()in thegetPinnedScriptsmethod.Minor cleanup:
java/src/org/openqa/selenium/JavascriptExecutor.java: Removed unnecessary import forCollections.Motivation and Context
I provide these minor changes to reduce compiler warnings and keep code cleaner and simplier
Types of changes
Checklist
PR Type
enhancement
Description
By,ByAll,ByChained) by usingthisinstead oftoString().JavascriptExecutorby replacingCollectors.toSet()withCollectors.toUnmodifiableSet().CollectionsinJavascriptExecutor.Changes walkthrough 📝
By.java
Improve exception message in `By` classjava/src/org/openqa/selenium/By.java
thisinstead oftoString().JavascriptExecutor.java
Enhance collection handling in `JavascriptExecutor`java/src/org/openqa/selenium/JavascriptExecutor.java
Collectors.toSet()withCollectors.toUnmodifiableSet().Collections.ByAll.java
Improve exception message in `ByAll` classjava/src/org/openqa/selenium/support/pagefactory/ByAll.java
thisinstead oftoString().ByChained.java
Improve exception message in `ByChained` classjava/src/org/openqa/selenium/support/pagefactory/ByChained.java
thisinstead oftoString().